filetransfer: handle exceptions thrown from enqueueItem#348
filetransfer: handle exceptions thrown from enqueueItem#348
Conversation
📝 WalkthroughWalkthroughAdded noexcept to FileTransfer::enqueueFileTransfer declarations and overrides; updated enqueue implementations to build local TransferItem(s), catch nix::Error on enqueue failures, mark items failed, and return ItemHandle without propagating exceptions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
| modifiedRequest.setupForS3(); | ||
| return enqueueItem(make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback))); | ||
| auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback)); | ||
| try { |
There was a problem hiding this comment.
It was suggested that I move this try-catch into enqueueItem directly, but I decided not to do that because I didn't want to mess with the fix in NixOS#14418
I tried a few different ways to reduce the duplication, but I couldn't figure out the best way to do that. ref<TransferItem> item; outside of the if doesn't work because there's no constructor, and I didn't want to copy request every time in the off chance that it's an s3 request...
Happy to take suggestions on improvements here.
There was a problem hiding this comment.
Please see my comment below (I should have posted it here). Refactoring is definitely doable if item is initialized once and unconditionally, meaning that its value should come from another function (make it inline if you want). No new copying is needed, the request is passed by reference and the callback is moved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libstore/filetransfer.cc`:
- Around line 968-989: The failure path in enqueueFileTransfer can return an
ItemHandle that refers to a destroyed TransferItem when enqueueItem throws;
update the failure return to transfer ownership so the returned ItemHandle owns
a ref to the item (e.g., change ItemHandle to hold a ref<Item> or add/use an
ItemHandle constructor that accepts the ref), and in both branches (the s3
modifiedRequest branch and the normal branch around enqueueItem)
construct/return the ItemHandle from the owning ref (move the ref from the local
variable into the ItemHandle) instead of creating an ItemHandle from a raw Item
reference; ensure item->fail(e) is called while the ref is still held so the
item remains alive for callers (e.g., unpause) after the function returns.
cole-h
force-pushed
the
filetransfer-enqueue-exceptions
branch
from
a6402da to
2208cf9
Compare
| } catch (const nix::Error & e) { | ||
| item->fail(e); | ||
| return ItemHandle(static_cast<Item &>(*item)); | ||
| } |
There was a problem hiding this comment.
That's a lot of repetition. Maybe this function should be refactored?
inline ref<TransferItem>
makeTransferItem(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept
{
/* Handle s3:// URIs by converting to HTTPS and optionally adding auth */
if (request.uri.scheme() == "s3") {
auto modifiedRequest = request;
modifiedRequest.setupForS3();
return make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
} else {
return make_ref<TransferItem>(*this, request, std::move(callback));
}
}
ItemHandle
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
{
const auto item = makeTransferItem(request, std::move(callback));
try {
return enqueueItem(item);
} catch (const nix::Error & e) {
item->fail(e);
return ItemHandle(static_cast<Item &>(*item));
}
}
It compiles, but please don't trust my choices about naming and inlining too much.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libstore/filetransfer.cc (1)
1063-1074:⚠️ Potential issue | 🟠 MajorSame
noexceptconcern applies here.
std::make_shared<std::promise<...>>()can throwstd::bad_alloc, but the function isnoexcept. If memory allocation fails,std::terminatewill be called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libstore/filetransfer.cc` around lines 1063 - 1074, The function FileTransfer::enqueueFileTransfer is marked noexcept but performs a potentially throwing allocation via std::make_shared<std::promise<FileTransferResult>>; remove the noexcept specifier from FileTransfer::enqueueFileTransfer (or otherwise ensure allocation cannot throw, e.g., use a preallocated promise or not allocate) so allocation failures do not call std::terminate; update the signature of enqueueFileTransfer and any callers if necessary to propagate exceptions normally when std::make_shared<std::promise<FileTransferResult>> throws.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/libstore/filetransfer.cc`:
- Around line 968-991: The function enqueueFileTransfer is marked noexcept but
operations like modifiedRequest.setupForS3(), make_ref<TransferItem>(...), and
the TransferItem constructor can throw, which would call std::terminate; to fix,
remove the noexcept specifier or wrap the entire function body in a try-catch
that catches std::exception (and ...), and in the catch create and return a
failed ItemHandle (or otherwise return an ItemHandle that signals failure) while
calling item->fail(e) when a TransferItem exists; specifically adjust the s3
branch (where setupForS3() is called) and the common path that constructs
make_ref<TransferItem> so exceptions are caught before escaping
enqueueFileTransfer and handled by returning a dummy/failed ItemHandle or
propagating safely if you choose to drop noexcept.
---
Outside diff comments:
In `@src/libstore/filetransfer.cc`:
- Around line 1063-1074: The function FileTransfer::enqueueFileTransfer is
marked noexcept but performs a potentially throwing allocation via
std::make_shared<std::promise<FileTransferResult>>; remove the noexcept
specifier from FileTransfer::enqueueFileTransfer (or otherwise ensure allocation
cannot throw, e.g., use a preallocated promise or not allocate) so allocation
failures do not call std::terminate; update the signature of enqueueFileTransfer
and any callers if necessary to propagate exceptions normally when
std::make_shared<std::promise<FileTransferResult>> throws.
| ItemHandle | ||
| enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override | ||
| { | ||
| /* Handle s3:// URIs by converting to HTTPS and optionally adding auth */ | ||
| if (request.uri.scheme() == "s3") { | ||
| auto modifiedRequest = request; | ||
| modifiedRequest.setupForS3(); | ||
| return enqueueItem(make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback))); | ||
| auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback)); | ||
| try { | ||
| return enqueueItem(item); | ||
| } catch (const nix::Error & e) { | ||
| item->fail(e); | ||
| return ItemHandle(ref<Item>(std::move(item))); | ||
| } | ||
| } | ||
|
|
||
| return enqueueItem(make_ref<TransferItem>(*this, request, std::move(callback))); | ||
| auto item = make_ref<TransferItem>(*this, request, std::move(callback)); | ||
| try { | ||
| return enqueueItem(item); | ||
| } catch (const nix::Error & e) { | ||
| item->fail(e); | ||
| return ItemHandle(ref<Item>(std::move(item))); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual function in the file to confirm its current state
head -1000 src/libstore/filetransfer.cc | tail -100Repository: DeterminateSystems/nix-src
Length of output: 3430
🏁 Script executed:
# Check if setupForS3 can throw nix::Error
rg -n 'setupForS3' --type cpp -B 2 -A 10Repository: DeterminateSystems/nix-src
Length of output: 6673
🏁 Script executed:
# Check for throw statements in s3-related code
rg -n 'throw' --type cpp -g '*s3*' | head -30Repository: DeterminateSystems/nix-src
Length of output: 1282
🏁 Script executed:
# Look for ParsedS3URL::parse implementation
rg -n 'ParsedS3URL::parse' --type cpp -B 2 -A 15Repository: DeterminateSystems/nix-src
Length of output: 7084
🏁 Script executed:
# Check if TransferItem constructor can throw
rg -n 'struct TransferItem\|class TransferItem' --type cpp -A 20Repository: DeterminateSystems/nix-src
Length of output: 52
noexcept may cause std::terminate if pre-enqueue operations throw.
The function is marked noexcept, but several operations before the try-catch blocks can throw:
modifiedRequest.setupForS3()(line 974) callsParsedS3URL::parse()which throwsBadURL(e.g., for invalid bucket names or missing keys)make_ref<TransferItem>(...)(lines 975, 984) can throwstd::bad_allocor exceptions from theTransferItemconstructor
If any of these throw, std::terminate will be called. Consider wrapping the entire function body in a try-catch, or removing noexcept if the exception guarantee cannot be upheld.
🛠️ Suggested fix: wrap entire body in try-catch
ItemHandle
enqueueFileTransfer(const FileTransferRequest & request, Callback<FileTransferResult> callback) noexcept override
{
+ try {
/* Handle s3:// URIs by converting to HTTPS and optionally adding auth */
if (request.uri.scheme() == "s3") {
auto modifiedRequest = request;
modifiedRequest.setupForS3();
auto item = make_ref<TransferItem>(*this, std::move(modifiedRequest), std::move(callback));
- try {
- return enqueueItem(item);
- } catch (const nix::Error & e) {
- item->fail(e);
- return ItemHandle(ref<Item>(std::move(item)));
- }
+ return enqueueItem(item);
}
auto item = make_ref<TransferItem>(*this, request, std::move(callback));
- try {
- return enqueueItem(item);
- } catch (const nix::Error & e) {
- item->fail(e);
- return ItemHandle(ref<Item>(std::move(item)));
+ return enqueueItem(item);
+ } catch (const nix::Error & e) {
+ // Need to handle the case where item doesn't exist yet
+ // This requires restructuring to ensure item is always created first
+ // or using a different error reporting mechanism
}
}Note: A complete fix requires restructuring to handle exceptions that occur before the TransferItem is created (e.g., in setupForS3()), since there's no item to mark as failed in that case. You may need to create a "dummy" failed item or reconsider the noexcept guarantee.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/libstore/filetransfer.cc` around lines 968 - 991, The function
enqueueFileTransfer is marked noexcept but operations like
modifiedRequest.setupForS3(), make_ref<TransferItem>(...), and the TransferItem
constructor can throw, which would call std::terminate; to fix, remove the
noexcept specifier or wrap the entire function body in a try-catch that catches
std::exception (and ...), and in the catch create and return a failed ItemHandle
(or otherwise return an ItemHandle that signals failure) while calling
item->fail(e) when a TransferItem exists; specifically adjust the s3 branch
(where setupForS3() is called) and the common path that constructs
make_ref<TransferItem> so exceptions are caught before escaping
enqueueFileTransfer and handled by returning a dummy/failed ItemHandle or
propagating safely if you choose to drop noexcept.
|
I've actually explored the approach with making the thing noexcept, but I think the double-callback issue was fully addressed with NixOS#14838. |
3 participants
Motivation
Context
Summary by CodeRabbit